Fix fractional scatter height with duplicated traces; re-enable scatter for HARPSPOL - #40
Open
astro-alexis wants to merge 3 commits into
Open
Fix fractional scatter height with duplicated traces; re-enable scatter for HARPSPOL#40astro-alexis wants to merge 3 commits into
astro-alexis wants to merge 3 commits into
Conversation
A fractional extraction_height is resolved against the median separation between adjacent traces. Trace.run stores `grouped + raw_traces`, so on an instrument with a fibers block every trace appears twice, and where a group holds a single fiber the two copies carry identical polynomials. Half the adjacent separations are then exactly 0, so the median is 0 and the aperture collapses to zero height: no trace is masked, and the polynomial is fitted to the order flux itself. On HARPSPOL blue this put the model 8.9x above the frame's own inter-order floor, identically at every extraction_height from 0.2 to 1.0 -- the insensitivity to height being the visible symptom. Extracted flux came out at 621.8 against 2027.8 uncorrected, with 27.3% of points negative; after the fix, 1988.5 and 0.41%. Ignore zero separations, and warn rather than silently falling back to a 10 px aperture. HARPS, UVES, XSHOOTER and LICK_APF have no fibers block, so they never duplicate traces and none of them were affected. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
ivh#38 dropped the scatter block and the scatter entry in get_expected_values, on the evidence that the step removed most of the stellar flux at every height tried. That measurement was taken with the pre-b6258b2 method (model fitted on LAMP,LAMP,TUN and subtracted unscaled) and with the spacing defect fixed in the previous commit, which disabled trace masking altogether. With both addressed, the mid-gap diagnostic from scatter.md gives model/floor of 1.18x on blue and 1.02x on red at 0.7 of the order spacing, from a sweep over 0.2 to 1.0 on gamma Equ (HD 201601, 2012-07-16). HARPS lands at 1.33x for comparison. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40 +/- ##
=========================================
Coverage ? 57.66%
=========================================
Files ? 58
Lines ? 8991
Branches ? 0
=========================================
Hits ? 5185
Misses ? 3806
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The [Unreleased] entry still said the HARPSPOL scatter step was withdrawn, which the previous two commits reverse. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #38 and the scatter work on master. Short version: the #39 conclusion that scatter is unusable for HARPSPOL reproduces exactly, but the cause was neither the method nor
extraction_height— it was a spacing defect that silently disabled trace masking. With it fixed, the step lands wherescatter.mdsays it should.The defect
estimate_background_scatterresolves a fractionalextraction_heightagainst the median separation between adjacent traces.Trace.runstoresgrouped + raw_traces(steps/trace.py:135), so on an instrument with afibersblock every trace appears twice — and where a group holds a single fiber, the merged and raw polynomials are identical. HARPSPOL blue: 90 physical traces, 180 stored.Half the adjacent separations are then exactly 0, so the median is 0,
xwdbecomes 0,half = 0,height = 0, and the aperture masks nothing. The polynomial is fitted to the order flux itself.HARPS, UVES, XSHOOTER and LICK_APF have no
fibersblock, so they never duplicate traces — which is why the four-instrument validation didn't surface this.Evidence
γ Equ (HD 201601), 2012-07-16, BLUE, one frame, 90 traces. Mid-gap diagnostic from
scatter.md(model ÷ the frame's own inter-order floor):extraction_heightThe flatness before the fix is the signature — height had no effect because masking never happened. End-to-end:
That first row pair is the #39 symptom: 69% of the flux gone, a quarter of points negative, insensitive to height.
Changes
estimate_background_scatter: measure spacing from non-zero separations; warn instead of silently using the 10 px fallback.test_scatter.py: regression test, verified to fail without the fix (28.0 vs 10.0).scatterrestored toget_expected_valuesandsettings.jsonatextraction_height: 0.7.Full unit suite passes (748, 1 skip for numba).
Note
The duplication is general, not HARPSpol-specific — any fiber instrument whose groups hold a single fiber will store every trace twice. This PR makes the scatter fit robust to it rather than changing what
Trace.runsaves, since the concatenation is deliberate and other steps rely on_select_traces. Worth a separate look at whether ANDES/MOSAIC hit anything else through the same path.🤖 Generated with Claude Code